#2144 — chore(repo): forbid tracked generated artifacts

Verdict: LGTM 🟢

Two things bundled cleanly:

Tracked-artifact guard. check-tracked-artifacts.mjs catches .DS_Store and node_modules that snuck into the index (3 producer .DS_Store files removed here). Runs in pre-commit (lefthook) AND in lint. The segments.includes("node_modules") check correctly operates on path segments, not substrings — the test at line 12 (docs/node_modules-policy.md → false) proves this. SSOT: FORBIDDEN_BASENAMES + the segment check is the single rule; .gitignore is the complementary prevent-at-add-time gate.

CI stacked-PR efficiency. Concurrency group regression-${{ github.event.pull_request.number || github.ref }} cancels in-progress runs when a PR rebases. Cache-to writer restricted to push (main) — PRs consume but don't write, preventing concurrent export exhaustion across the 31-PR stack. Smart prep.

No SSOT violations. Clean.

Review by Miga

Final direct pass at current head 585aa9f. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.

Reviewed at 585aa9f6.

Clean chore. Enforcement is two distinct execution layers — pre-commit (lefthook.yml:41 tracked-artifacts hook) + CI (package.json lint script now prefixed with bun run check:tracked-artifacts). Not illusory defense-in-depth: the pre-commit catches locally-forgotten stagings before CI ever sees them, and CI catches anyone who bypassed hooks. Deleted .DS_Stores look load-bearing-free (binary tracked cruft, nothing imports them).

A few small things worth naming:

• Regression concurrency group is scoped correctly. regression-${{ github.event.pull_request.number || github.ref }} at regression.yml:14 means force-pushes AND Graphite base updates on the same PR share a group → newer run cancels older. On push events (main), github.ref is the fallback — each ref gets its own group; concurrent main pushes would cancel each other, which is fine because the cache write is what matters and only the newest push should win.
• Single-writer cache discipline at regression.yml:120 (cache-to: ${{ github.event_name == 'push' && '...' || '' }}) — empty string on PRs is treated as unset by buildx cache. PRs get cache-from for warmth but never write, so N stacked branches can't stomp on each other. Right call.
• isForbiddenTrackedPath handles Windows separators. normalized = filePath.replaceAll("\\", "/") at check-tracked-artifacts.mjs:8, and the test at .test.mjs:8 exercises packages\\producer\\node_modules\\pkg. Since git ls-files -z emits forward slashes on all platforms, the normalization only matters for direct callers of the helper (currently just tests), but the safety-net is cheap and correct.
• Nit (non-blocking): lint now hard-depends on git ls-files succeeding. Any context that runs bun run lint without a git-visible working tree (rare — e.g., a Docker image where .git isn't copied in) will fail with a git ls-files exited with status N from check-tracked-artifacts.mjs:20. Not gating for any known caller, but worth naming if a downstream image build ever mounts source without .git.

LGTM from my side.

— Review by Rames D Jusso

Verdict: 🟢 LGTM at 585aa9f

Compact repo-hygiene PR. Endorses Miga's + Rames-D's take — pre-commit + lint are two distinct enforcement layers, not illusory defense-in-depth. One scope-drift note added on top of what the prior reviewers surfaced.

Findings

F1. Cache-writer narrowing on regression workflow — verified safe. docker/build-push-action@v6 parses cache-to via getInputList (newline/comma split with .filter(Boolean)), so the yaml expression ${{ ... && '<scope>' || '' }} at .github/workflows/regression.yml:120 collapses to zero exports on PRs — no literal '' scope name leak. Main pushes remain the single writer. Correct pattern.

F2. Concurrency group — no merge_group collision. regression-${{ github.event.pull_request.number || github.ref }} at .github/workflows/regression.yml:13. Triggers are only pull_request: and push: branches: main (regression.yml:17-21); no merge_group/merge_queue events are wired in this repo. PR runs group by number, main runs by refs/heads/main, no cross-run cancellation.

F3. check-tracked-artifacts.mjs scope narrower than title (nit). Script guards only .DS_Store basenames + any path segment named node_modules (scripts/check-tracked-artifacts.mjs:5,10). Title reads "forbid tracked generated artifacts" (plural, generic) — accurate reading is closer to "forbid .DS_Store and node_modules". Not a defect; the check works and the segment-vs-substring distinction is correctly tested at check-tracked-artifacts.test.mjs:12 per Miga's cite. Title-tightening is a REST PATCH away, not a new commit.

F4. FORBIDDEN_BASENAMES extensibility. new Set([".DS_Store"]) is a small starter set. Follow-on candidates that a future author might reach for: .AppleDouble, ._*, .Thumbs.db, dist/, .next/, .turbo/, coverage/. Not blocking — this PR ships the guard mechanism; content expansion is cheap follow-up work.

Adversarial pass

Initial instinct: flag F3 as request-changes for scope-drift. Downgraded to nit because the current implementation is correct and the title-fix is trivial. Also considered flagging the cache-writer narrowing as a scope-drift (title doesn't mention CI cache) — dropped since it lives in the same file as the .DS_Store deletions and is adjacent hygiene work, well-commented in the workflow with intent.

R1 by Via

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

Reviewed the PR’s own final-head diff against 2aadf450, not the cumulative Graphite stack. No new findings beyond the already-recorded git-less-lint/title-scope nits.

• scripts/check-tracked-artifacts.mjs:8-27 uses path segments/basenames and NUL-delimited git ls-files, so similarly named source files do not false-positive and index errors fail loud.
• .github/workflows/regression.yml:13-15,123 scopes cancellation per PR/ref and leaves PRs cache-read-only while main remains the only writer.
• The head is a single direct commit on its recorded base; no later stack payload leaked into this boundary.

Verification: 4/4 checker unit tests plus the live repository check passed locally; all 54 final-head GitHub check runs completed without failure.

Verdict: COMMENT (post-merge: Ready)
Reasoning: The incremental boundary is clean, the repository invariant and cache single-writer policy behave as documented, and no post-merge follow-up defect was found.

— Deepwork